feat: DC-5512 Update navbar per website #7149
Conversation
carlagn
requested review from
Techboidesign,
aidankmcalister,
ankur-arch,
mhessdev,
nikolasburk and
nurul3101
WalkthroughRefactors navbar layout and responsive styles, shifts homepage breakpoints, adds a left-nav separator element, updates DocSearch styling and injects a Kapa button into the DocSearch modal that forwards queries to window.Kapa, and defines a dark-theme brand color variable. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Page
participant DocSearch as DocSearch Modal
participant KapaBtn as Injected Kapa Button
participant Kapa as window.Kapa
User->>Page: Open search (shortcut/click)
Page->>DocSearch: Initialize & open modal
Page->>DocSearch: Inject styles and insert KapaBtn
User->>DocSearch: Type query
DocSearch->>KapaBtn: input event → update label/query
User->>KapaBtn: Click Kapa button
KapaBtn->>DocSearch: Close modal
KapaBtn->>Kapa: window.Kapa.open({ query, submit: true })
Kapa-->>User: Kapa UI opens with query
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
Dangerous URL checkNo absolute URLs to prisma.io/docs found. |
Redirect checkThis PR probably requires the following redirects to be added to static/_redirects:
|
Deploying docs with
|
| Latest commit: |
12bdcd0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c865fe00.docs-51g.pages.dev |
| Branch Preview URL: | https://feat-dc-5512-new-navbar.docs-51g.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/theme/Navbar/Content/styles.module.css (1)
13-17: Improve non-interactive separator UXAdd minor affordances to avoid accidental selection and align baseline.
.separator { color: var(--border-color); font-size: 28px; margin-right: 4px; + user-select: none; + display: inline-block; + line-height: 1; }src/css/custom.css (2)
519-521: Hover indicator: add keyboard focus parity + avoid event interception
- Mirror the hover bar for
:focus-visibleto meet focus visibility requirements.- Ensure the decorative
::afterdoesn’t intercept pointer events.- The
position: relative;in the hover rule is redundant since it’s set on.navbar__link.-.navbar__item.navbar__link:not(.logo-link):not(.navbar-login-btn):hover { - position: relative; -} - -.navbar__item.navbar__link:not(.logo-link):not(.navbar-login-btn):hover::after { +.navbar__item.navbar__link:not(.logo-link):not(.navbar-login-btn):hover::after, +.navbar__item.navbar__link:not(.logo-link):not(.navbar-login-btn):focus-visible::after { position: absolute; height: 2px; width: 100%; content: ""; background: var(--surface-brand-default); bottom: -24px; left: 0; z-index: 2; + pointer-events: none; }Also applies to: 523-536
798-800: Nested selector in .css and mobile “/”
- This uses nesting (
&::before) in a .css file; the repo already relies on PostCSS nesting, but calling it out to avoid accidental regressions if tooling changes.- Consider
aria-hidden="true"on the wide-screen slash (addressed in TSX) and keep this one purely visual.Confirm PostCSS nesting is enabled in the build (it’s used elsewhere in this file too).
src/pages/index.module.scss (1)
9-11: Unify 996px/997px breakpoints across the codebaseThe recent scan shows many remaining
@media (min-width: 997px)alongside the new 996px rules—this 1px offset can cause layout gaps around 996–997px. Extract a single breakpoint variable and replace all occurrences:
- In your global SCSS (e.g.
src/theme/_variables.scss), define$bp-lg: 996px;- Update media queries to use
(min-width: $bp-lg)or(max-width: $bp-lg).- Find leftovers with:
rg -nP '@media\s*\((min|max)-width:\s*(?:997|996)px\)'- Incrementally replace:
@media (min-width: 997px)→@media (min-width: $bp-lg)@media (min-width: 996px)/@media (max-width: 996px)→(min|max)-width: $bp-lgThis centralizes your breakpoint and eliminates any 1px mismatches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/css/custom.css(4 hunks)src/pages/index.module.scss(10 hunks)src/theme/Navbar/Content/index.tsx(1 hunks)src/theme/Navbar/Content/styles.module.css(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Check internal links
- GitHub Check: runner / linkspector
- GitHub Check: Lost Pixel
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
src/theme/Navbar/Content/styles.module.css (1)
8-10: Good: hide separator on small viewportsHiding the visual-only separator below 996px aligns with the mobile “/” added via CSS on the link. LGTM.
src/css/custom.css (2)
674-679: Navbar layout tweaks: verify no regression around 996–1191px
- Switching middle items to flex and changing grid columns/gaps is fine; just validate alignment and overflow at boundary widths, especially with the new 48px row height and
.navbar::afterspacer.Quick checks:
- 996px, 997px, 1191px widths: ensure the hover bar (bottom: -24px) sits above
.navbar::afterand doesn’t clip.- Tokens
--navbar-items-bgexist in both themes.Also applies to: 682-687, 692-694, 701-715
515-517: Scope active link styles to navbar only
The rule at src/css/custom.css (around lines 513–516) currently matches both.navbar__link--activeand.menu__link--active, affecting docs sidebar. If unintended, restrict to:-.navbar__link--active, -.menu__link--active { +.navbar .navbar__link--active { color: var(--surface-brand-default); background: var(--surface-brand-grey-strong); }Then add a sidebar-specific rule that sets both
colorandbackgroundfor.menu__link--active(the existing sidebar selector only definesbackground).src/theme/Navbar/Content/index.tsx (1)
75-76: Make slash decorative and correct docs link
- Add
aria-hidden="true"to the separator span to hide it from screen readers.- Point the “docs” link at the actual docs base (e.g.
/docs) usinguseBaseUrl('/docs'), either inline or via aconst docsUrl = useBaseUrl('/docs');.- Request: confirm your docs plugin’s
routeBasePath(default/docs) indocusaurus.config.jsso the link stays correct across deployments.- <span className={styles.separator}>/</span> - <Link to={baseUrl} className="logo-link">docs</Link> + <span className={styles.separator} aria-hidden="true">/</span> + <Link to={useBaseUrl('/docs')} className="logo-link">docs</Link>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/css/custom.css(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: runner / linkspector
- GitHub Check: Cloudflare Pages
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/css/custom.css (1)
806-808: Typo:.navbar__items--midlewon’t match.Should be
--middle. Current rule never applies on mobile.- .navbar__items--midle { + .navbar__items--middle { height: fit-content; }
🧹 Nitpick comments (5)
src/css/custom.css (5)
513-517: Scope active styles to navbar to avoid sidebar coupling.This rule also targets
.menu__link--active; later sidebar rules override it, but relying on cascade is brittle. Prefer scoping the background to navbar only.-.navbar__link--active, -.menu__link--active { +.navbar__link--active { color: var(--surface-brand-default); background: var(--surface-brand-grey-strong); } +/* Sidebar keeps its existing behavior via the later-specific rules */
540-541: Verify hit target after padding reduction.Padding is now 8px; ensure the clickable area remains at least 44px tall per accessibility guidance. If not, add a min-height.
.navbar__item { padding: 8px; + min-height: 44px; }
674-679: Layout shift to flex/grid looks good; consider variable gaps.Switch to flex + gap and grid columns reads well. Consider tying
gap: 40px/16pxto a token (e.g.,var(--space-4)/--space-2) to keep spacing consistent across breakpoints.Also applies to: 682-682, 685-687, 692-694
707-707: Avoid fixed height on responsive row.
height: 48pxrisks clipping when items wrap/localize. Prefer min-height and rely on alignment in the flex container.- height: 48px; + min-height: 48px;Also applies to: 713-713
799-801: Slash separator fix landed; add spacing/visual parity.Good fix moving to
.logo-link::before. Add spacing and color to match the desktop.separator..logo-link::before { - content: "/" + content: "/"; + display: inline-block; + margin: 0 4px 0 8px; /* tweak as needed to match .separator */ + color: var(--border-color); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/css/custom.css(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Check internal links
- GitHub Check: runner / linkspector
- GitHub Check: Lost Pixel
🔇 Additional comments (1)
src/css/custom.css (1)
887-887: Border-bottom replacement LGTM.Cleaner than box-shadow and aligns with the site’s border tokens.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/css/custom.css (1)
523-536: Make hover underline accessible and drop redundant rule.
- Include keyboard focus parity; underline should appear on focus-visible too.
- Remove redundant
position: relative;on the hover rule; base.navbar__linkalready sets it.- Ensure the pseudo-element is non-interactive.
-.navbar__item.navbar__link:not(.logo-link):not(.navbar-login-btn):hover { - position: relative; -} - -.navbar__item.navbar__link:not(.logo-link):not(.navbar-login-btn):hover::after { +.navbar__item.navbar__link:not(.logo-link):not(.navbar-login-btn):is(:hover, :focus-visible)::after { position: absolute; height: 2px; width: 100%; content: ""; background: var(--surface-brand-default); bottom: -24px; left: 0; z-index: 2; + pointer-events: none; }Also applies to: 519-521
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/css/custom.css(5 hunks)src/theme/SearchBar/index.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: runner / linkspector
🔇 Additional comments (6)
src/css/custom.css (5)
513-517: Verify active-state token contrast.Check that
--surface-brand-defaulton--surface-brand-grey-strongmeets WCAG 2.1 AA (≥4.5:1 for normal text) in light and dark themes.
674-687: LGTM: navbar middle group layout aligns with design goals.Flex + gap and grid-template adjustments look consistent with the new navbar structure.
692-694: LGTM: responsive gap reduction at ≤1191px.Tighter spacing at this breakpoint improves fit without impacting readability.
701-721: Mobile stacking: validate overlap with navbar bottom bar.At ≤996px,
.navbar__itemsheight is 48px and.navbar__innerswitches to flex; also.navbar::aftercreates a 48px bar (lines 1624–1633). Please verify no double spacing/overlap occurs with dropdowns or the search container.
799-801: Good fix: proper pseudo-element selector.Using
.logo-link::before(non-nested) under the media query resolves the prior invalid nesting issue; the separator should now render on mobile.src/theme/SearchBar/index.tsx (1)
57-57: Verify token definition and contrast
Confirm--surface-brand-defaultis declared in every theme’s stylesheet and that its contrast against the button text meets WCAG AA standards.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/theme/SearchBar/index.tsx (2)
38-42:contextualSearchshould be boolean, not string.Used in a boolean conditional later; mismatched type may cause config/runtime surprises.
type DocSearchProps = Omit<DocSearchModalProps, "onClose" | "initialScrollY"> & { - contextualSearch?: string; + contextualSearch?: boolean; externalUrlRegex?: string; searchPagePath: boolean | string; };
80-103:.kapa-ai-iconusescontent: url(...)on a normal element — ineffective.
contentapplies to pseudo‑elements. Usebackgroundor an actual<img>/<picture>; or switch to::before.Option A (background image on the element):
-[data-theme='light'] .kapa-ai-icon { - content: url('https://www.prisma.io/docs/img/logo-dark.svg'); - width: 32px !important; - height: 32px !important; - max-width: 32px !important; - max-height: 32px !important; - object-fit: cover !important; - object-position: 0 0 !important; -} +[data-theme='light'] .kapa-ai-icon { + background: url('https://www.prisma.io/docs/img/logo-dark.svg') no-repeat 0 0 / cover !important; + width: 32px !important; + height: 32px !important; +}Option B (pseudo‑element):
.kapa-ai-icon::before { content: url('...'); display: inline-block; width:32px; height:32px; }Also, this class isn’t used in the injected HTML; remove dead CSS if not needed.
♻️ Duplicate comments (1)
src/theme/SearchBar/index.tsx (1)
141-174: Flatten nested selectors in runtime CSS (hover + kbd).This repeats earlier feedback; injected strings shouldn’t rely on CSS Nesting.
.DocSearch-Button { @@ - &:hover { - background: var(--surface-primary); - outline: 1px solid var(--border-color); - } - kbd { - background: var(--white-color); - color: var(--gray-800); - box-shadow: 0px 1px 1px rgba(47, 55, 71, 0.6), 0px 1px 4px rgba(47, 55, 71, 0.2); - border-bottom: 1px solid rgba(47, 55, 71, 0.2); - border-radius: 4px; - padding: 0; - vertical-align: baseline; - font-size: 14px !important; - font-family: "JetBrainsMono" !important; - } } +.DocSearch-Button:hover { + background: var(--surface-primary); + outline: 1px solid var(--border-color); +} +.DocSearch-Button kbd { + background: var(--white-color); + color: var(--gray-800); + box-shadow: 0px 1px 1px rgba(47, 55, 71, 0.6), 0px 1px 4px rgba(47, 55, 71, 0.2); + border-bottom: 1px solid rgba(47, 55, 71, 0.2); + border-radius: 4px; + padding: 0; + vertical-align: baseline; + font-size: 14px !important; + font-family: "JetBrainsMono" !important; +}
🧹 Nitpick comments (4)
src/theme/SearchBar/index.tsx (4)
319-323: Removeas anyon ref; use precise nullable type.No need to wait for React 19; this is valid today.
-// TODO remove "as any" after React 19 upgrade -const searchButtonRef = useRef<HTMLButtonElement>(null as any); +const searchButtonRef = useRef<HTMLButtonElement | null>(null);
49-60: Tidy.kapa-ai-buttonstyles: duplicate/contradictory width and invalid border value.
width: 100%conflicts withwidth: fit-content(keep the latter).border: transparent;is invalid; useborder: 0or set border-color explicitly.- width: 100%; - margin: 0 0 0 auto; - width: fit-content; - border: none; - background: transparent; font-size: 14px; + margin-left: auto; + width: fit-content; + border: 0; + background: transparent; + font-size: 14px; @@ - border: transparent;
388-426: Effect cleanup for input listener is misplaced; ensure removal on close/unmount.Returning from inside
setInterval’s callback doesn’t register as an effect cleanup. Move listener teardown to the effect’s returned function.- React.useEffect(() => { + React.useEffect(() => { if (!isOpen) return; - // Wait for the search modal to be fully rendered - const checkForSearchBox = setInterval(() => { + let removeInputListener: (() => void) | undefined; + const checkForSearchBox = setInterval(() => { const searchBox = document.querySelector(".DocSearch-Form"); const searchInput = document.querySelector(".DocSearch-Input"); if (searchBox && searchInput instanceof HTMLInputElement) { clearInterval(checkForSearchBox); // If there's already text in the search input, use it if (searchInput.value) { insertKapaButton(searchInput.value); } // Listen for input events directly const handleInputChange = () => { if (searchInput.value) { insertKapaButton(searchInput.value); } }; // Add input event listener for real-time updates searchInput.addEventListener("input", handleInputChange); - - // Clean up this listener when the modal closes or component unmounts - return () => { - searchInput.removeEventListener("input", handleInputChange); - }; + removeInputListener = () => searchInput.removeEventListener("input", handleInputChange); } }, DOM_CHECK_INTERVAL_MS); - // Clean up interval if component unmounts or modal closes - return () => { + return () => { clearInterval(checkForSearchBox); + removeInputListener?.(); }; }, [isOpen, insertKapaButton]);Optional: replace polling with a MutationObserver watching
.DocSearch-Modalsubtree to reduce wakeups.
368-371: Localize button copy (“Ask AI about this topic”).Use Docusaurus
<Translate>or reuse translations to keep i18n consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/theme/SearchBar/index.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: runner / linkspector
- GitHub Check: Check internal links
- GitHub Check: Lost Pixel
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
src/theme/SearchBar/index.tsx (1)
366-371: No action needed: Font Awesome CSS is already globally loaded
Verified./src/css/all.cssis listed indocusaurus.config.ts(line 234).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/theme/SearchBar/index.tsx (1)
397-435: Input listener never cleaned up → leak across modal opens.Returning a cleanup function inside
setInterval’s callback doesn’t register with React; the input listener persists across mounts. Abort or explicitly remove in the effect’s cleanup.- React.useEffect(() => { - if (!isOpen) return; - - // Wait for the search modal to be fully rendered - const checkForSearchBox = setInterval(() => { - const searchBox = document.querySelector(".DocSearch-Form"); - const searchInput = document.querySelector(".DocSearch-Input"); - - if (searchBox && searchInput instanceof HTMLInputElement) { - clearInterval(checkForSearchBox); - - // If there's already text in the search input, use it - if (searchInput.value) { - insertKapaButton(searchInput.value); - } - - // Listen for input events directly - const handleInputChange = () => { - if (searchInput.value) { - insertKapaButton(searchInput.value); - } - }; - - // Add input event listener for real-time updates - searchInput.addEventListener("input", handleInputChange); - - // Clean up this listener when the modal closes or component unmounts - return () => { - searchInput.removeEventListener("input", handleInputChange); - }; - } - }, DOM_CHECK_INTERVAL_MS); - - // Clean up interval if component unmounts or modal closes - return () => { - clearInterval(checkForSearchBox); - }; - }, [isOpen, insertKapaButton]); + React.useEffect(() => { + if (!isOpen) return; + + const abort = new AbortController(); + const checkForSearchBox = setInterval(() => { + const searchBox = document.querySelector(".DocSearch-Form"); + const searchInput = document.querySelector(".DocSearch-Input"); + + if (searchBox && searchInput instanceof HTMLInputElement) { + clearInterval(checkForSearchBox); + + if (searchInput.value) insertKapaButton(searchInput.value); + + const handleInputChange = () => { + if (searchInput.value) insertKapaButton(searchInput.value); + }; + + // Add listener with AbortSignal so cleanup is automatic + searchInput.addEventListener("input", handleInputChange, { signal: abort.signal }); + } + }, DOM_CHECK_INTERVAL_MS); + + return () => { + abort.abort(); + clearInterval(checkForSearchBox); + }; + }, [isOpen, insertKapaButton]);
♻️ Duplicate comments (4)
src/css/custom.css (1)
519-536: Accessible focus parity for navbar underline is now in place.
:is(:hover, :focus-visible)::afterandpointer-events: noneaddress the prior keyboard/hover parity and hit‑testing concerns. Looks good.src/theme/SearchBar/index.tsx (3)
10-12: React imports fixed (TS-friendly).Using
import React, { type ReactNode, ... }pluscreatePortalresolves the prior build break.
153-183: Runtime CSS nesting removed for DocSearch button/keyboard hints.Selectors are flattened (
:hover,kbd), which ensures cross‑browser behavior.
115-126: Flatten nested&:beforein injected CSS (invalid in plain CSS).The template string still uses Sass-style nesting; browsers will ignore it. Flatten to a sibling
::beforerule.-.DocSearch-Modal:not(:has(.DocSearch-Dropdown)) .DocSearch-Footer { - margin-top: 200px; - position: relative; - &:before { - content: "No recent searches"; - position: absolute; - top: -100px; - color: var(--tertiary-font-color); - left: 50%; - transform: translateX(-50%); - } -} +.DocSearch-Modal:not(:has(.DocSearch-Dropdown)) .DocSearch-Footer { + margin-top: 200px; + position: relative; +} +.DocSearch-Modal:not(:has(.DocSearch-Dropdown)) .DocSearch-Footer::before { + content: "No recent searches"; + position: absolute; + top: -100px; + color: var(--tertiary-font-color); + left: 50%; + transform: translateX(-50%); +}
🧹 Nitpick comments (3)
src/css/custom.css (2)
701-708: Prefer min-height for resilient mobile header.Hard-coding
height: 48px;can clip items if the locale increases label length or system font sizes are larger. Considermin-height: 48px;instead.- height: 48px; + min-height: 48px;
799-801: Mobile “/” separator renders; consider spacing and non-interactive behavior.Add spacing and explicitly disable hit‑testing to avoid caret jumps on selection.
- .logo-link::before { - content: "/" - } + .logo-link::before { + content: "/"; + display: inline-block; + margin: 0 8px 0 4px; + color: var(--border-color); + pointer-events: none; + }src/theme/SearchBar/index.tsx (1)
45-61: Minor CSS tidy-ups in injected styles.Remove redundant/invalid declarations to keep the stylesheet lean.
.kapa-ai-button { - display: flex; - align-items: center; - width: 100%; - margin: 0 0 0 auto; - width: fit-content; + display: flex; + align-items: center; + margin: 0 0 0 auto; + width: fit-content; border: none; - background: transparent; font-size: 14px; + background: transparent; + font-size: 14px; color: var(--surface-brand-default); cursor: pointer; transition: all 0.2s ease; position: relative; overflow: hidden; - border: transparent; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/css/custom.css(5 hunks)src/theme/SearchBar/index.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: runner / linkspector
- GitHub Check: Check internal links
- GitHub Check: Lost Pixel
🔇 Additional comments (5)
src/css/custom.css (5)
674-687: Grid → Flex swap: verify dropdown stacking and wrap behavior at narrow widths.Switching
.navbar__items--middleto flex with a 16px gap is fine; just confirm items don’t wrap unexpectedly around the center at 1–2 extra nav items and that the underline::after(z-index: 2) doesn’t overlay the dropdown menu. If needed, addz-indexto the dropdown container.
689-695: Tighten horizontal rhythm on mid breakpoints.Gap reduction reads well; no issues found.
710-714: Mobile layout: explicit flex is OK.Switch to flex on
.navbar__innerfor mobile aligns with the center/right split. No problems spotted.
717-721: Absolutely positioned right slot: check overflow on small devices.
top: 100%andwidth: calc(100% - 16px)work; ensure long placeholder text within the search/button doesn’t overflow the viewport (addmax-width: 100%on inner button/input if needed).
883-888: Border-bottom replaces shadow: aligns with the site’s flat header spec.No issues.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/css/custom.css(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: runner / linkspector
- GitHub Check: Check internal links
- GitHub Check: Lost Pixel
- GitHub Check: Cloudflare Pages
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/css/custom.css (4)
518-525: Remove redundant positioning on the hover rule
.navbar__linkis alreadyposition: relative;, so the extra hover-only block is unnecessary. Trim it to reduce specificity noise..navbar__link { position: relative; } - -.navbar__item.navbar__link:not(.logo-link):not(.navbar-login-btn):hover { - position: relative; -}
526-535: Good a11y underline; consider adding :focus fallbackThe combined
:is(:hover, :focus-visible)+pointer-events: noneis solid. For older Safari users who lack:focus-visible, consider including:focusas a non-breaking fallback.-.navbar__item.navbar__link:not(.logo-link):not(.navbar-login-btn):is(:hover, :focus-visible)::after { +.navbar__item.navbar__link:not(.logo-link):not(.navbar-login-btn):is(:hover, :focus, :focus-visible)::after { position: absolute; /* … */ }
723-727: Positioning may rely on implicit positioning — make it explicitYou set
left: 0; top: 100%; width: calc(100% - 16px);but nopositionhere. If the last child isn’t already positioned, these offsets won’t apply. Recommend making it explicit to avoid regressions when the child’s default changes..navbar__items--right > :last-child { + position: absolute; /* if intended to drop below the bar */ left: 0; z-index: 1; top: 100%; width: calc(100% - 16px); }Confirm the intended containing block.
.navbar__innerisposition: relative;at Line 717, so the absolute child should anchor to it.
805-807: Fix applied for ::before — add minor polishSwitching from nested
&::beforeto.logo-link::beforeunder the media query fixes the earlier break. Add a trailing semicolon and a bit of spacing so the slash doesn’t collide with the logo.@media (max-width: 996px) { .logo-link { /* … */ } .logo-link::before { - content: "/" + content: "/"; + display: inline-block; + margin-right: 8px; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/css/custom.css(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Lost Pixel
- GitHub Check: runner / linkspector
- GitHub Check: Check internal links
🔇 Additional comments (4)
src/css/custom.css (4)
513-516: Scoped active-state fix looks goodLimiting the background to navbar links resolves the sidebar bleed-through. Nice catch.
539-548: Padding shrink: check pointer target size on desktopPadding is now 8px. On mobile you set 48px height (Line 713), which is great. On wider viewports, please ensure practical hit area remains comfortable (e.g., ~40–44px line box) given the underline bar sitting at -24px.
700-707: Transform reset across breakpointsThe reset to
transform: unset;on mobile undoes the desktop lift. Looks intentional and consistent with the new layout.
893-894: Navbar divider addition looks correctBorder aligns with the design shift from shadow to separator; mobile keeps its own background below.
Fixes #DC-5512
Summary by CodeRabbit
New Features
Style